Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(companions-pane): Add new companions pane to saved trip editor and account settings screen #1289

Merged
merged 37 commits into from
Nov 19, 2024

Conversation

josh-willis-arcadis
Copy link
Contributor

Description:
Add ui for trusted companion to the saved trip editor and account settings screens.
Sketch can be found here: https://www.sketch.com/s/3e1c091e-4fb3-4e51-8f17-86be7a8c7b1b

PR Checklist:

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of comments. I have requested a change on the backend for sending emails to folks added as companions, so we can test the whole cycle.

@@ -48,6 +49,13 @@ const ExistingAccountDisplay = (props: Props) => {
<FormattedMessage id="components.MobilityProfile.MobilityPane.header" />
)
},
{
collapsible: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an interesting feature that was in the mockup, although I'm split on whether to have it for this pane. I'll let the other reviewer weigh in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not the biggest fan of it here. I think this is a nice to have feature, but it doesn't make sense if this is the only section that uses it. If all the sections were collapsable I think this would make more sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

lib/components/user/common/add-email-form.tsx Outdated Show resolved Hide resolved
lib/components/user/existing-account-display.tsx Outdated Show resolved Hide resolved
lib/components/user/mobility-profile/companions-pane.tsx Outdated Show resolved Hide resolved
lib/components/user/mobility-profile/companions-pane.tsx Outdated Show resolved Hide resolved
lib/components/user/standard-panes.tsx Outdated Show resolved Hide resolved
lib/components/user/monitored-trip/saved-trip-screen.js Outdated Show resolved Hide resolved
lib/components/user/monitored-trip/saved-trip-editor.tsx Outdated Show resolved Hide resolved
lib/components/user/types.ts Outdated Show resolved Hide resolved
@josh-willis-arcadis josh-willis-arcadis force-pushed the trusted-companions branch 2 times, most recently from 27da499 to 600fd50 Compare October 22, 2024 18:14
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few tweaks for now, but I want to wait for backend fixes before I can test confirmed companions.

lib/components/util/status-badge.tsx Outdated Show resolved Hide resolved
lib/components/user/types.ts Outdated Show resolved Hide resolved
lib/components/util/status-badge.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor tweaks and we can proceed. Looking good otherwise.

lib/components/user/mobility-profile/companions-pane.tsx Outdated Show resolved Hide resolved
lib/components/util/status-badge.tsx Outdated Show resolved Hide resolved
i18n/en-US.yml Outdated Show resolved Hide resolved
i18n/en-US.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that the section should not be collapsible. Otherwise code looks good, haven't tested the actual interface yet

@josh-willis-arcadis
Copy link
Contributor Author

The collapsible prop has been removed from the companions pane now.

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and interface both look great!

@josh-willis-arcadis josh-willis-arcadis merged commit 3749ee6 into dev Nov 19, 2024
9 checks passed
@josh-willis-arcadis josh-willis-arcadis deleted the trusted-companions branch November 19, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants